-
Notifications
You must be signed in to change notification settings - Fork 22
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
chore(ci): various makefile improvements #3410
Merged
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Signed-off-by: John Cowen <[email protected]>
✅ Deploy Preview for kuma-gui ready!
To edit notification comments on pull requests, go to your Netlify site configuration. |
johncowen
force-pushed
the
chore/mk-imp
branch
4 times, most recently
from
January 16, 2025 16:40
1a124e6
to
53fc3d6
Compare
Signed-off-by: John Cowen <[email protected]>
johncowen
force-pushed
the
chore/mk-imp
branch
from
January 16, 2025 16:55
53fc3d6
to
c3ac66c
Compare
schogges
approved these changes
Jan 16, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM 👍
Signed-off-by: John Cowen <[email protected]>
schogges
approved these changes
Jan 17, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
👍
johncowen
added a commit
that referenced
this pull request
Jan 20, 2025
Follow up of #3410 --- Turns out `npm query` requires your `node_modules` to be present, I've no idea why 🤷 , and we have a target that we sometimes run that needs to run before a `make install` (`make release/prune`) 🤦 . I saw that you can add a `npm --package-lock-file-only query` to get it to use the lock file instead of needing access to `node_modules`. This works when running from workspace root without `node_modules`, but when you run a Makefile from a sub project without `node_modules` adding this arg/flag still fails 🤷 I've fallen back to just jq-ing the package-lock.json myself at least for the moment so we can unblock the release workflow. If I find a slightly nicer way to do this in the future I'll replace, but I really want to stick to the runtime resolution of things rather than hardcoding paths in several files. How to test: - Run `make clean` to remove all your `node_modules` folders - cd into or use `make -C` to run `make release/prune` from within `packages/kuma-gui` - Verify that `packages/config` has changes to package.json and package-lock.json also has changes. I 'think' this will finally unblock the release workflow 🤞 --- edit: I just checked a this new approach actually works better elsewhere also 🎉 Signed-off-by: John Cowen <[email protected]>
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
I stumbled across quite a few things whilst fixing up our
clean
script so:make install
)I have a longer term goal of making our "wiring root" Makefiles easier to maintain (there might end up being lots of them) and possibly creating them from a template. This goal means that the less path differences etc (
../
over here,./
over there) the better. This has influenced some decisions here.In order to run
make clean
from anywhere and it automatically do this from root, we need to know where root is, from wherever we are. This led me to usingnpm prefix
forNPM_WORKSPACE_ROOT
. This led me to realize aNPM_WORKFLOW_CONFIG_PATH
that I'd not noticed in a previous PR, which led me to ourrelease
script. Getting here made me realize ourrelease
script wasn't working since we moved ourmk
files over to@kumahq/config
.Hopefully the above explains why this is "various" things rather than just the one.
make release/prune
fixesSince we moved
mk
to@kumahq/config
, and we alsorm
@kumahq/config
during SBOM generation, this means we can no longer run any Makefiles after SBOM generation, doh 🤦I figured that we only want to delete the dependency configuration for the SBOM generation, not the entire
@kumahq/config
module. Additionally our SBOM generation ignores devDependencies, and@kumahq/config
is a devDependency but its dependencies are not (ideally our SBOM generator would ignore devDependencies entirely but hey ho).This PR runs a tiny script before SBOM generation that moves
@kumahq/config
s package config fordependencies
todevDependencies
and also deletes itspeerDependencies
. We then runnpm install --package-lock-only
as before. This I believe has the exact same effect asrm
ing the entire @kumahq/config` module, but without also deleting our Makefile tooling/scripts.make clean
Can now be run from any repository in the workspace or from the root. I also added a confirmation (if not in CI) so that you have some kind of visibility into what its deleting and a chance to say no.
make variables
I added some useful variables into
help.mk
, which are necessary for all scripts. I added them tohelp.mk
because I would like to make it easy to add thehelp
make target and make it hard not to. Adding them to make means you must includehelp.mk
copypasta ability
I made it so the top ~11 lines of every
Makefile
are exactly the same, this makes it easy to copy/paste them to make multiple ones and keep them in sync i.e. poor mans templating (we may add templating at some point but I doubt it)wait for a system/application to test
Lastly I added a wait for our test site to come up, cypress wasn't waiting long enough and even before this it would try 3 times before (sometimes) succeeding.
make
now tales a split second longer due to the new variables, so I added a quick check for our application being served on localhost:5681 before running Cypress.I may add some inlines.